-
Notifications
You must be signed in to change notification settings - Fork 637
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(compare_map_segmentation): add missing mutex lock #9097
base: main
Are you sure you want to change the base?
fix(compare_map_segmentation): add missing mutex lock #9097
Conversation
Signed-off-by: badai-nguyen <[email protected]>
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
@badai-nguyen |
e448739
to
0d6d565
Compare
Signed-off-by: badai-nguyen <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9097 +/- ##
==========================================
- Coverage 28.26% 27.33% -0.94%
==========================================
Files 1306 1304 -2
Lines 101091 95745 -5346
Branches 39243 39036 -207
==========================================
- Hits 28573 26168 -2405
+ Misses 69761 66968 -2793
+ Partials 2757 2609 -148
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
perception/autoware_compare_map_segmentation/src/distance_based_compare_map_filter/node.cpp
Outdated
Show resolved
Hide resolved
...eption/autoware_compare_map_segmentation/src/voxel_grid_map_loader/voxel_grid_map_loader.hpp
Outdated
Show resolved
Hide resolved
Isn't is necessary to take a mutex here?
|
Signed-off-by: badai-nguyen <[email protected]>
Signed-off-by: badai-nguyen <[email protected]>
...ption/autoware_compare_map_segmentation/src/voxel_distance_based_compare_map_filter/node.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: badai-nguyen <[email protected]>
7bed7df
to
061359e
Compare
perception/autoware_compare_map_segmentation/src/distance_based_compare_map_filter/node.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: badai-nguyen <[email protected]>
Signed-off-by: badai-nguyen <[email protected]>
Signed-off-by: badai-nguyen <[email protected]>
...eption/autoware_compare_map_segmentation/src/voxel_grid_map_loader/voxel_grid_map_loader.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: badai-nguyen <[email protected]>
Strictly speaking, relaxed memory ordering is enough here (I think), but stricter ordering will not invoke a bug, so I will admit it.
After you check the result of Autoware Evaluator again, I will approve this PR!! |
@veqcc I recheck this update on Evaluator again and all tests were passed. https://evaluation.tier4.jp/evaluation/reports/1e5dd036-27fa-5480-b44f-b4ef14d6aa3d?project_id=prd_jt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@yukkysaito Could you review this PR as codeowner? |
The code itself seems fine. However, I haven't thoroughly checked whether the resources being locked are appropriate. |
Yes it is. |
Description
current_voxel_grid_dict_
which was pointed out here: feat(compare_map_segmentation): add dynamic map loading for voxel_based_compare_map_filter #3087 (comment)static_loader
anddynamic_loader
mode to reduce contention and more understandable.mutex
from constructorsRelated links
Parent Issue:
How was this PR tested?
Tested in Autoware Evaluator
Notes for reviewers
None.
Interface changes
None.
Effects on system behavior
None.